Use 'instanceandevents' endpoint when updating process state #937
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| else | ||
| { | ||
| _logger.LogError($"Unable to update instance process with instance id {instance.Id}"); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 4 days ago
To fix the problem, we need to sanitize the instance.Id before logging it. This can be done by removing any newline characters from the instance.Id to prevent log forging. We will use the String.Replace method to achieve this.
- Identify the logging statement that uses
instance.Id. - Sanitize the
instance.Idby removing newline characters before logging it. - Ensure that the fix does not change the existing functionality of the code.
| @@ -189,3 +189,4 @@ | ||
| { | ||
| _logger.LogError($"Unable to update instance process with instance id {instance.Id}"); | ||
| string sanitizedInstanceId = instance.Id.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", ""); | ||
| _logger.LogError($"Unable to update instance process with instance id {sanitizedInstanceId}"); | ||
| throw await PlatformHttpException.CreateAsync(response); |
…a new instance....
|
| @@ -683,6 +691,10 @@ [FromRoute] Guid instanceGuid | |||
| ProcessChangeResult startResult = await _processEngine.GenerateProcessStartEvents(processStartRequest); | |||
|
|
|||
| targetInstance = await _instanceClient.CreateInstance(org, app, targetInstance); | |||
| foreach (var instanceEvent in startResult.ProcessStateChange?.Events ?? []) | |||
There was a problem hiding this comment.
These loops are needed because GenerateProcessStartEvents is creating the events, at which point InstanceId is null..
There was a problem hiding this comment.
Was this a bug before this fix?
There was a problem hiding this comment.
I don't think so, as it started to happen after switching to the new endpoint.. Don't know why, will do some digging to make sure
Description
Uses the new endpoint which commits new process state and instance events in one transaction.
The related tests were a little funny. Test names said they were testing more than they actually were, it seems like most of the variation comes from the
AltinnTaskTypethingy.Related Issue(s)
InstanceEvents) within a single transaction altinn-storage#544Verification
Documentation